Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CDConfig: Add cd_content for file templating #61

Merged
merged 3 commits into from
May 26, 2021
Merged

CDConfig: Add cd_content for file templating #61

merged 3 commits into from
May 26, 2021

Conversation

saleemrashid
Copy link
Contributor

Implement cd_content from hashicorp/packer#10859, allowing file templating similar to http_content.

There currently aren't any tests for this feature. I'd like to first make the cd_files tests more robust (at the moment they only test that there are the expected number of keys in filesAdded), and then add tests for cd_content (including when used alongside cd_files).

@saleemrashid saleemrashid requested a review from a team as a code owner April 29, 2021 18:04
@hashicorp-cla
Copy link

hashicorp-cla commented Apr 29, 2021

CLA assistant check
All committers have signed the CLA.

Comment on lines +68 to +83
// Key/Values to add to the CD. The keys represent the paths, and the values
// contents. It can be used alongside `cd_files`, which is useful to add large
// files without loading them into memory. If any paths are specified by both,
// the contents in `cd_content` will take precedence.
//
// Usage example (HCL):
//
// ```hcl
// cd_files = ["vendor-data"]
// cd_content = {
// "meta-data" = jsonencode(local.instance_data)
// "user-data" = templatefile("user-data", { packages = ["nginx"] })
// }
// cd_label = "cidata"
// ```
CDContent map[string]string `mapstructure:"cd_content"`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Love this 👍🏼

Copy link
Contributor

@azr azr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This LGTM, super nice, thanks, waiting for #62 to be fixed/merged first before running manual tests.

@azr
Copy link
Contributor

azr commented May 17, 2021

Hi there @saleemrashi, #62 got merged, do you have the time to rebase & test this ? Thanks !

@saleemrashid saleemrashid changed the title [WIP] CDConfig: Add cd_content for file templating CDConfig: Add cd_content for file templating May 17, 2021
@saleemrashid
Copy link
Contributor Author

@azr Rebased and added tests 🙏 (and caught the missing os.MkdirAll in AddContent by means of writing those tests 😄)

@saleemrashid
Copy link
Contributor Author

(Sorry, pressed the button by accident 😄)


func (s *StepCreateCD) AddContent(dst, path, content string) error {
// Join Cleans the path so we can join it without path traversal issues.
dstPath := filepath.Join(dst, path)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed the filepath.Clean call as Join already does that ! :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@azr From testing, doing filepath.Join("/folder", "../hello") would result in /hello which I don't think would be intended behaviour. Doing filepath.Clean on an absolute path would turn ../hello into /hello so it joins to /folder/hello.

Copy link
Contributor

@azr azr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested this a little; this looks great to me ! Thanks !

@azr azr merged commit 2da2de8 into hashicorp:main May 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants